Skip to content

Toby cookbook-axes #172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 10, 2015
Merged

Toby cookbook-axes #172

merged 8 commits into from
Mar 10, 2015

Conversation

tdhock
Copy link
Contributor

@tdhock tdhock commented Feb 28, 2015

This branch translates some of Marianne's visual tests from the axes.R file in the add-r-cookbook-tests branch

https://github.com/ropensci/plotly/blob/add-r-cookbook-tests/tests/cookbook-test-suite/axes.R

to the format I need for adding them to the test table.

@tdhock
Copy link
Contributor Author

tdhock commented Feb 28, 2015

TODO: add tests in R code for things like font color.

@chriddyp
Copy link
Member

chriddyp commented Mar 2, 2015

nice! should we just add the cookbook .R files all in one PR (so that we include them in our visual tests) and then do fixes on separate subsequent PRs?

@tdhock
Copy link
Contributor Author

tdhock commented Mar 2, 2015

well I was thinking of proposing 1 PR per cookbook .R file, and adding fixes in the same PR along the way, so that master is always passing checks on travis.

@chriddyp
Copy link
Member

chriddyp commented Mar 3, 2015

OK, cool, that makes sense!

@tdhock
Copy link
Contributor Author

tdhock commented Mar 4, 2015

Hey @cpsievert I did git pull origin master from this branch, and I was expecting that travis would run and redo the test table, but it does not seem to be running. Any idea why? If you know how to fix it then go ahead and push to this branch.

@cpsievert
Copy link
Collaborator

I think we just have to be patient. Travis is still building...

@cpsievert
Copy link
Collaborator

It's up now -- http://ropensci.github.io/plotly-test-table/

@tdhock
Copy link
Contributor Author

tdhock commented Mar 5, 2015

awesome, but indeed there are some missing images in the current test table

http://ropensci.github.io/plotly-test-table/tables/00a5c8df16e2b19a6629c32ed42ef3a76d6fbac6/index.html

so I restarted the build via the travis web page like you suggested.

however I anticipate this will be an issue for almost all future builds, for example there is one missing image in your new branch's test table

http://ropensci.github.io/plotly-test-table/tables/f26e22467e4ad91b367bd5b15d9f68f304a7a4f9/density.html

From the plotly side it would be nice if @chriddyp could investigate and see why the plotly web site randomly returns an error when we ask for a png image.

From the R side I will attempt to modify the plotly-test-table code so that it checks for missing images and attempt to re-download them.

@cpsievert
Copy link
Collaborator

httr could help on the R side. Instead of these lines, you could do something like

library(httr)
g <- GET(plotly.png.url, write_disk(plotly.png.file))
warn_for_status(g)
# if we want to stop the build, use stop_for_status() instead

@tdhock
Copy link
Contributor Author

tdhock commented Mar 5, 2015

thanks for the suggestion, I will look into that.

by the way after re-building the table it seems that the missing PNGs now are true negatives (the ggplotly function stops with an error for master, but works for this branch)

@chriddyp
Copy link
Member

chriddyp commented Mar 5, 2015

warn_for_status is great, the status code will be really helpful in debugging. my guess is that it is a timeout issue, but I'm not sure without seeing the status code. we could also put in a retry logic: if it failed, pause for 5 seconds, and then re-try.

@chriddyp
Copy link
Member

chriddyp commented Mar 9, 2015

Hey @tdhock and @cpsievert - what do you guys think about merging in all the cookbook .R files (without additional fixes) so that we can get a broader set of tests in our test-table before we merge other PRs like #178 and #167 (and risk degradation!)

@tdhock
Copy link
Contributor Author

tdhock commented Mar 9, 2015

I still think it makes more sense to do one cookbook .R file at a time, to avoid having the tests on master failing. but if you think it is better to avoid regressions I can do that (and maybe if we want to avoid having the Travis master test fail, we can just add the image tests to the table without adding any expect_ calls in R code).

@tdhock
Copy link
Contributor Author

tdhock commented Mar 9, 2015

by the way I think we can merge this branch right away, here is the test table

http://ropensci.github.io/plotly-test-table/tables/07af388fed4266ed8a87fb48ddb711aefcfa7655/index.html

coord.lim <- p$coord$limits[[xy]]
if(is.numeric(coord.lim)){
## TODO: maybe test for more exotic coord specification types
## involving NA, Inf, etc?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add this as a github issue so that we can keep track of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added #184

@chriddyp
Copy link
Member

other than my comments, looks good to me 👍

tdhock pushed a commit that referenced this pull request Mar 10, 2015
@tdhock tdhock merged commit 3eda7bc into master Mar 10, 2015
@tdhock tdhock deleted the toby-cookbook branch March 10, 2015 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants